fix: correct data corruption and MuData compatibility bugs - #1
Merged
Conversation
Findings from a repository audit. Each fix is pinned by a regression test that fails without it. Data correctness: - Categorical columns were relabelled by position when a category was unused, so e.g. "c" was read back as "b". Decode against the full category list instead of truncating it to the number of observed codes. - NA in character columns was coerced to the literal string "NaN". Store those columns as categorical (missing = code -1), as anndata has no nullable-string encoding. - WriteH5AD fell through to the first assay when `assay` did not match any name, silently writing the wrong modality. It now errors instead. - The single-layer fallback always wrote `counts`, because is.null() was applied to the list rather than to its elements. A data-only assay failed. MuData compatibility: - /obsm and /varm were created lazily inside the reductions loop, and /varp, /obsmap and /varmap were never written at all, so files without multimodal reductions could not be opened by mudata. All of the groups readers require are now always written. - ReadH5MU used names(modalities)[2:length()], which yields c(NA, ...) for a single modality. Also: use nchar() rather than length() when stripping modality prefixes from reduction names, report the anndata format version (not the mudata one) in the anndata userblock, and drop unreachable branches, a dead obs-name prefix guard, and four unused writers. Tests: - Add tests/testthat.R. Without it R CMD check never ran the suite, and test_sparse.R had been failing unnoticed since the default sparse.type became csr_matrix. Corrected those expectations and extended coverage to metadata, assay selection and .h5mu round-trips (57 assertions). Docs: - Regenerate man pages, which omitted the scale.data and sparse.type arguments, and correct CITATION.cff, which described muon. Verified against anndata 0.12 and mudata 0.3.1. R CMD check is clean.
|
Analysis CompleteGenerated ECC bundle from 1 commits | Confidence: 50% View Pull Request #2Repository Profile
Changed Files (15)
Top hotspots
Top directories
Analysis Depth Readiness (commit-history, 7%)ECC Tools uses this to decide whether recommendations should stay at commit-history/setup guidance or expand into CI, security, harness, reference-set, AI-routing, and team backlog work.
Reference Set Readiness (0/7, 0%)
Generated Instincts (17)
After merging, import with: Files
|
There was a problem hiding this comment.
Pull request overview
This PR fixes several correctness and format-compliance issues in MuDataSeurat’s .h5ad/.h5mu read/write paths, and adds regression tests to prevent silent data corruption and MuData incompatibilities from reoccurring.
Changes:
- Fix categorical decoding and NA handling to avoid silent metadata corruption on round-trips.
- Make
.h5muoutput MuData-compatible by always writing required top-level groups (includingobsmap/varmap) and fix single-modality.h5mureading. - Add a full
testthatharness plus new regression tests covering sparse storage types, metadata, assay selection, and.h5muround-trips.
Reviewed changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test_sparse.R | Updates sparse-matrix round-trip tests for CSR/CSC behavior and error handling. |
| tests/testthat/test_metadata.R | Adds regression tests for factor/character metadata round-trips (unused levels, NA handling). |
| tests/testthat/test_assays.R | Adds tests for assay selection errors, layer handling, and .h5mu required groups/maps. |
| tests/testthat.R | Adds the testthat entrypoint so R CMD check runs the suite. |
| README.md | Documents new correctness guarantees and fixes usage examples. |
| R/WriteUtils.R | Fixes anndata userblock versioning and improves NA handling for character vectors via categorical encoding. |
| R/WriteH5MU.R | Fixes assay selection behavior, reduction-name prefix stripping, and ensures MuData-required groups/maps are written. |
| R/ReadUtils.R | Fixes categorical decoding to preserve full category lists (incl. unused levels) and corrects warning text. |
| R/ReadH5MU.R | Fixes single-modality modality iteration bug ([-1] vs 2:length()). |
| man/WriteH5MU.Rd | Regenerated docs for new args; contains minor parameter-doc typos (flagged). |
| man/WriteH5ADHelper.Rd | Regenerated docs to include new args. |
| man/WriteH5AD.Rd | Regenerated docs for new args; contains minor parameter-doc typos (flagged). |
| DESCRIPTION | Adds testthat/fs to Suggests and updates RoxygenNote. |
| CITATION.cff | Updates citation metadata to match this package instead of muon. |
| .Rbuildignore | Ignores CITATION.cff in R builds. |
Files not reviewed (3)
- man/WriteH5AD.Rd: Generated file
- man/WriteH5ADHelper.Rd: Generated file
- man/WriteH5MU.Rd: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+283
to
+290
| if (length(object@assays) > 1) { | ||
| h5$close() | ||
| stop(paste0( | ||
| "An assay to be written has to be provided, one of: ", | ||
| paste(names(object@assays), collapse = ", "), | ||
| ".\nUse WriteH5MU() to write all the modalities." | ||
| )) | ||
| } |
Comment on lines
+292
to
300
| } else if (!assay %in% names(object@assays)) { | ||
| # Never fall back to another assay: writing a different assay than the one | ||
| # that was asked for would silently produce a file with the wrong data. | ||
| h5$close() | ||
| stop(paste0( | ||
| "An assay to be written has to be provided, one of: ", | ||
| "Assay ", assay, " not found. Available assays: ", | ||
| paste(names(object@assays), collapse = ", "), | ||
| ".\nUse WriteH5MU() to write all the modalities." | ||
| "." | ||
| )) |
Comment on lines
+59
to
+62
| test_that("a csr_matrix .h5ad can be read", { | ||
| srt <- ReadH5AD(fileh5ad_r) | ||
| counts <- GetAssayData(srt, layer = "counts") | ||
|
|
Comment on lines
+71
to
+73
| test_that("a csc_matrix .h5ad can be read", { | ||
| srt <- ReadH5AD(fileh5ad_c) | ||
| counts <- GetAssayData(srt, layer = "counts") |
Comment on lines
351
to
+354
| names(var_names) <- modalities | ||
| write_data_frame(h5, "var", do.call(c, var_names)) | ||
|
|
||
| write_mod_maps(h5, modalities, nrow(obs), var_names) |
Comment on lines
+29
to
+31
| \item{scale.data}{Boolen, wether to export scale.data.} | ||
|
|
||
| \item{sparse.type}{String, save as csr_matrix or csc_matrix.} |
Comment on lines
+33
to
+36
| \item{scale.data}{Boolen, wether to export scale.data.} | ||
|
|
||
| \item{sparse.type}{String, save as csr_matrix or csc_matrix.} | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes found during a repository audit. Every fix is pinned by a regression test that fails without it.
Why this matters
No
.h5mufile this package produced could be opened by Pythonmudata.WriteH5MUcreated/obsmand/varmlazily inside the reductions loop, so an object without multimodal reductions never got them, and/varp,/obsmapand/varmapwere never written at all.obsmap/varmapturn out to be part of MuData format0.1.0— the version we already declare — so this was incompleteness rather than a version mismatch.Separately, two bugs silently corrupted metadata rather than failing loudly.
Data correctness
"c"was read back as"b". Cell-type/cluster labels corrupted.NAin character columns coerced toNaN"NaN". Now stored ascategorical(missing = code-1), since anndata has no nullable-string encoding.WriteH5ADfell through to assay #1 on an unmatchedassaywhich(!is.null(x))appliedis.null()to the list, not its elementscounts; a data-only assay errored out.MuData compatibility
obsm,varm,obsp,varp,obsmapandvarmap.ReadH5MUusednames(modalities)[2:length()], which yieldsc(NA, ...)for a single modality, so single-modality.h5mufiles could not be read.Tests
tests/testthat.Rwas missing, soR CMD checknever ran the suite. As a resulttest_sparse.Rhad been failing unnoticed since the defaultsparse.typebecamecsr_matrix(it still assertedcsc_matrix); those expectations are corrected and both storage paths are now covered. Coverage extended to metadata, assay selection and.h5muround-trips.0 → 57 assertions.
Other
nchar()rather thanlength()when stripping modality prefixes from reduction names.R/shrank by ~30 lines.scale.dataandsparse.typeentirely.CITATION.cffdescribed muon, not this package.Verification
R CMD check: Status: OK (0 errors / warnings / notes),[ FAIL 0 | WARN 0 | SKIP 0 | PASS 57 ].anndata 0.12.0rc1andmudata 0.3.1: NA, unused categories and matrix orientation all survive the round-trip.Notes for review
is("obs", "data.frame")guard was dead (it tested a string literal). It was removed rather than repaired — activating it would breakReadH5MU, which intersects obs names across modalities, so per-modality prefixes would empty the intersection.WriteH5ADandWriteH5MU, and the read path still returns a v3-styleAssayrather thanAssay5. Both are behaviour/refactor decisions rather than bug fixes.